docs(babysit-pipeline): correct the backfill guidance, retry the driver's fetch - #10660
Conversation
…ished run #10628 was written while the backfill was still running. Finishing it falsified three of its claims. - Halt-on-cluster counted raw failed `Generate:` runs. Since #10627 an impossible pair spends three runs on its own retries, so two bad pairs trip a threshold of five while nothing is wrong — observed exactly that (6 failures, 2 pairs, healthy pipeline). Count distinct (spec, library) pairs instead. - "Static library against an interactive or 3D spec is the one gap that is usually real" is replaced by its opposite. Every category-level prediction the backfill made was wrong: 17 of 20 parked pairs generated fine, pygal and chartjs each succeeded on the plot types they had been written off for, and bar-3d-categorical succeeded in plotnine while scatter-3d did not. Three pairs failed under a full budget — plotnine on scatter-3d, contour-3d and line-3d-trajectory, all needing a spatial projection plotnine lacks. A gap is real when three attempts say so, not when the pairing sounds implausible. - The one-retry rule needs a precondition now: the workflow already spends three attempts per campaign, so check the run list before adding a manual one. Three failures minutes apart is a measured gap; one is a flake. Same check resolves `RESULT=TIMEOUT` with `recent generate failures: 0`, which only means they aged out of the driver's 25-minute window. New gotcha: intersect label-derived spec ids with the real `plots/` directories. 14 of 26 ids harvested from `impl:*:failed` issue titles had no spec on main, and such a dispatch dies at "Validate specification exists". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RbZuWNDFy7kjXh9kLfA4dP
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RbZuWNDFy7kjXh9kLfA4dP
There was a problem hiding this comment.
🟡 Changes recommended
The updated skill text includes a misleading “filtered to the spec” instruction for gh run list that should be clarified to avoid incorrect operator usage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the babysit-pipeline skill documentation to correct gap-backfill guidance based on the completed 2026-08-24 backfill run, and records the correction in CHANGELOG.md.
Changes:
- Adjusted the “halt-on-cluster” guidance to count distinct
(spec, library)pairs rather than raw failedGenerate:runs. - Replaced capability “prediction” guidance with a “measure, don’t predict” rule and added a gotcha about spec IDs harvested from issue titles not necessarily existing on
main. - Added a corresponding
[Unreleased]changelog entry describing the corrected guidance.
File summaries
| File | Description |
|---|---|
CHANGELOG.md |
Adds an [Unreleased] entry documenting the corrected gap-backfill guidance. |
.claude/skills/babysit-pipeline/SKILL.md |
Updates the skill’s gap-backfill gotchas and halt/deferral guidance to match observed backfill behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - **A spec id from an issue title may not exist.** `impl:*:failed` | ||
| labels outlive their specs: of 26 spec ids harvested that way, 14 |
There was a problem hiding this comment.
Applied — "spec IDs". Matches the repo's existing usage (3 occurrences of "spec IDs" against the 1 I introduced) and the Google style guide this repo follows.
| manual retry adds nothing. `gh run list --workflow=impl-generate.yml` | ||
| filtered to the spec tells you which case you are in — three | ||
| `Generate: <lib> for <spec>` failures minutes apart is a measured |
There was a problem hiding this comment.
Fixed — the sentence implied a flag that does not exist. It now shows the actual command, filtering the output by run title:
gh run list --workflow=impl-generate.yml --limit 40 \
--json conclusion,createdAt,displayTitle \
--jq '.[] | select(.displayTitle | test("<spec>")) | "\(.createdAt) \(.conclusion) \(.displayTitle)"'…vers Two drivers polling the same checkout collide on .git/refs/remotes/origin/main: error: cannot lock ref 'refs/remotes/origin/main': is at 6b666c1 but expected c736a84 A lost fetch leaves origin/main stale, so meta_present understates what has landed and the poller reports PARTIAL for libraries that are already merged. Retry three times with backoff, and log a warning when all three lose instead of quietly polling a stale ref. Surfaced by the two-slot backfill this guidance introduced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RbZuWNDFy7kjXh9kLfA4dP
There was a problem hiding this comment.
🟡 Changes recommended
The PR description claims “docs-only” despite an executable script change, and the new fetch-retry logic should retain failure details in logs to keep diagnosis actionable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.claude/skills/babysit-pipeline/run_spec.sh:91
fetch_mainsuppresses allgit fetchstderr (2>/dev/null), so when the retries fail you only get a generic warning with no clue whether it was a ref-lock collision vs. an auth/network problem. Logging the last error (or per-attempt errors) toLOGwould make this diagnosable without spamming stdout.
git -C "$REPO" fetch origin main --quiet 2>/dev/null && return 0
sleep $(( i * 3 ))
done
echo "[warn] git fetch origin main failed 3x — origin/main may be stale this round" | tee -a "$LOG"
return 1
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| # Two drivers polling the same checkout collide on `.git/refs/remotes/origin/main` | ||
| # ("cannot lock ref ... is at X but expected Y"). A lost fetch leaves origin/main | ||
| # stale, which makes meta_present understate progress — so retry, and say so in | ||
| # the log when all three attempts lose, rather than silently polling a stale ref. | ||
| fetch_main() { |
There was a problem hiding this comment.
Right, and my fault: the script commit came after I wrote the description. Title and body updated — the scope line now reads "prose plus one executable change" and there is a dedicated section for the run_spec.sh behaviour change with its own verification.
On the log detail: fetch_main now captures git stderr and prints it with the warning. A ref-lock collision and an auth or network failure call for different responses, so discarding the message threw away the one bit that distinguishes them. Exercised against a bad ref → last error: fatal: couldn't find remote ref ….
- `gh run list` has no per-spec filter; show the jq title filter instead of a sentence that reads like a flag exists. - Keep git's stderr in fetch_main's warning: a ref-lock collision and an auth or network failure need different responses, and swallowing the message hides which one happened. - "spec ids" → "spec IDs", matching the repo's Google-style usage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RbZuWNDFy7kjXh9kLfA4dP
MarkusNeusinger
changed the title
There was a problem hiding this comment.
🔵 Needs a closer look
The updated “halt-on-cluster” bullet is currently phrased in a self-contradictory way (run-count threshold vs. “count pairs”), which could cause incorrect operational decisions during a backfill.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.claude/skills/babysit-pipeline/SKILL.md:111
- The halt-on-cluster bullet mixes a run-count threshold ("≥5 failed Generate runs") with the instruction "Count pairs, not runs", which is internally contradictory and easy to misapply during an outage. Reword the condition to make it explicit that the decision is driven by distinct failing (spec, library) pairs, with the run count as supporting evidence.
- **Halt the whole queue** only on a failure CLUSTER — ≥5 failed
`Generate:` runs in minutes across **≥3 distinct (spec, library)
pairs** = model daily quota exhausted; a fallback model via
`-f model=` can finish leftovers. Count pairs, not runs: one
genuinely impossible pair burns three runs on its own retries, so
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
The bullet kept a "≥5 failed runs" clause and then said "count pairs, not runs" — contradictory, and the kind of contradiction that gets resolved the wrong way during an actual outage. The condition is now just the pair count, with the run count named as the thing that does NOT decide it and the reason why. Reported by the Copilot reviewer on #10660. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RbZuWNDFy7kjXh9kLfA4dP
| - **Halt the whole queue** only on a failure CLUSTER (≥5 failed | ||
| `Generate:` runs in minutes = model daily quota exhausted; a | ||
| fallback model via `-f model=` can finish leftovers). | ||
| - **Halt the whole queue** only on a failure CLUSTER: **≥3 distinct |
There was a problem hiding this comment.
Agreed, and worth fixing rather than explaining away — a bullet that says "≥5 runs" and "count pairs, not runs" in the same breath gets resolved the wrong way precisely when someone is reading it under pressure.
The condition is now just the pair count:
Halt the whole queue only on a failure CLUSTER: ≥3 distinct (spec, library) pairs failing within minutes of each other = model daily quota exhausted […] The threshold is pairs, full stop — the raw run count says nothing, because one impossible pair spends three runs on its own auto-retries, so two bad pairs already produce six failures with the pipeline entirely healthy (observed 2026-08-24).
The run count now appears only as the thing that does not decide it.
There was a problem hiding this comment.
🔵 Needs a closer look
fetch_main returns non-zero but is called unguarded (can become fatal under bash -e/inherited set -e), and there’s a newly introduced “spec id” casing inconsistency in SKILL.md.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
.claude/skills/babysit-pipeline/run_spec.sh:97
fetch_mainreturns non-zero after logging, but it is invoked as a bare command. If the script is ever executed with-eenabled (e.g.,bash -e run_spec.shorset -einherited from a parent shell), this will abort the whole driver on a transient ref-lock collision—the opposite of the intended “warn and keep polling stale origin/main for this round” behavior.
This issue also appears on line 126 of the same file.
fetch_main
.claude/skills/babysit-pipeline/run_spec.sh:126
- Same as above: calling
fetch_mainunguarded means a non-zero return can terminate the loop early when-eis in effect. If the goal is to keep polling even when the fetch lost 3x, make this call non-fatal.
fetch_main
.claude/skills/babysit-pipeline/SKILL.md:223
- Use the conventional uppercase acronym “ID” in this heading sentence for consistency with the surrounding text (and with earlier corrections): “A spec ID from an issue title may not exist.”
- **A spec id from an issue title may not exist.** `impl:*:failed`
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Why
#10628 documented gap backfill while the backfill was still running. Finishing it falsified three of its claims, so this PR corrects them rather than leaving confident wrong guidance in a skill. Running the backfill in two parallel slots also exposed a race in the driver, fixed here.
Scope: prose plus one executable change.
SKILL.mdandCHANGELOG.mdare documentation;run_spec.shgains a fetch retry (behaviour change, details below).Corrections
1. Halt-on-cluster counted the wrong thing. The threshold was "≥5 failed
Generate:runs in minutes = quota exhausted". Since #10627 restored the full retry budget, a single impossible pair spends three runs on its own auto-retries — so two bad pairs trip a raw count of five while the pipeline is perfectly healthy. That happened during the run: 6 failures at 22:29–22:37, all of themplotnineon two specs, nothing wrong with the pipeline. Now counts distinct(spec, library)pairs.2. "Static library + interactive/3D spec is the one gap that is usually real" was backwards. Every category-level prediction made during the backfill turned out wrong:
network-force-directed,arc-basic, and every JS-block specbar-3d-categoricalsucceeded;scatter-3ddid notmap-marker-clusteredandline-stress-strain17 of 20 parked pairs generated fine. Exactly three failed under a full budget — plotnine on
scatter-3d,contour-3d,line-3d-trajectory, all of which need a spatial projection plotnine does not have, while the "3D" spec representable in 2D went through. The gotcha now says: measure, don't predict.3. The one-retry rule needed a precondition. The workflow now spends three attempts per campaign by itself, so a pair that comes back missing may already be measured and the manual retry adds nothing. The skill now shows the actual command —
gh run listhas no per-spec filter, so it filters the output by run title with jq. Three failures minutes apart is a gap, one is a flake. The same check resolvesRESULT=TIMEOUTwithrecent generate failures: 0, which only means the failures aged out of the driver's 25-minute window (seen online-3d-trajectory, which had in fact failed three times 40 minutes earlier).New gotcha
Spec IDs harvested from
impl:*:failedissue titles must be intersected with the realplots/directories: 14 of 26 pointed at specs that no longer exist on main, and such a dispatch dies seconds in atValidate specification exists. This is how the rescue list first read as 45 missing implementations when only 20 were real.Behaviour change:
run_spec.shretries its fetchTwo drivers polling the same checkout collide on the ref lock:
A lost fetch leaves
origin/mainstale, someta_presentunderstates what has landed and the poller reportsPARTIALfor libraries that are already merged — a failure that disguises itself as a stalled spec.fetch_main()now retries three times with backoff and, when all three lose, logs the warning with git's own stderr so a ref-lock collision is distinguishable from an auth or network failure.Verification
bash -nonrun_spec.sh; the failure path exercised against a bad ref →last error: fatal: couldn't find remote ref …, confirming the message survives to the log.agentic/runs/babysit-2026-08-20/.Related
Previous failures for plotnine/contour-3d since 2026-08-24T10:38:22Z: 2followed by3 failed attempt(s) in the last 12h (cap: 3 per campaign).daily-regen liveness: workflow state is 'disabled_manually', not active — rescue skipped, scan completing successfully.🤖 Generated with Claude Code
https://claude.ai/code/session_01RbZuWNDFy7kjXh9kLfA4dP